[SPARK-16200][SQL] Rename AggregateFunction#supportsPartial #13852
Conversation
| * Indicates if this function needs to aggregate values group-by-group in a single step. | ||
| * If false, we must always use a `SortAggregateExec` operator without partial aggregates. | ||
| */ | ||
| def supportsPartial: Boolean = true |
There was a problem hiding this comment.
We'd be also better to rename this variable instead of supportsPartial because it's kinds of misleading; forceSortAggregate, needsSequentialAggregate, ...
There was a problem hiding this comment.
I do like the names, they describe however the inverse of what this flag does. We could invert the flag.
There was a problem hiding this comment.
okay. Since the suggested fix changes the code, I'll create a new jira ticket and attach this pr with it.
|
@hvanhovell ping |
|
Test build #61056 has finished for PR 13852 at commit
|
|
@hvanhovell ping |
| /** | ||
| * Indicates if this function supports partial aggregation. | ||
| * Currently Hive UDAF is the only one that doesn't support partial aggregation. | ||
| * Indicates if this function needs to aggregate values group-by-group in a single step. |
There was a problem hiding this comment.
This is also the inverse.
maropu
changed the title
|
Test build #61215 has finished for PR 13852 at commit
|
|
@hvanhovell ping |
|
are we overloading the semantics? I think it's actually useful to have a supportsPartial, which is what this was for. |
|
you mean we need two funcs: |
|
Do we have two requirements here? One is whether an aggregate function supports partial aggregation, and the other is whether the order should be enforced right ? |
|
Yes. Currently, we have three functions with |
|
okay, thanks! |
| override def inputTypes: Seq[AbstractDataType] = Seq(AnyDataType) | ||
|
|
||
| override def supportsPartial: Boolean = false | ||
| override def forceSortAggregate: Boolean = true |
There was a problem hiding this comment.
oh, after changing this name, it will not show that we do not partial agg for this function.
There was a problem hiding this comment.
yea. either way, it seems partial agg. becomes meaningless in future.
5 participants
What changes were proposed in this pull request?
Update a doc because it's stale.
This is a trivial fix, so I didn't create a JIRA ticket.
This fix was suggested in #13802 by @hvanhovell.
How was this patch tested?
N/A